Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Refactor Stateful operator and custom op #6928

Merged
merged 19 commits into from
Jul 12, 2017
Merged

Conversation

piiswrong
Copy link
Contributor

@piiswrong piiswrong commented Jul 5, 2017

@tqchen @eric-haibin-lin

  • Refactored CreateLayerOp to return any instead of Operator*.
  • Use nnvm interface instead of Operator::Forward/Backward
  • Added kLocal exec_type to run operators in main thread without pushing to engine.
  • Refactored Custom op to use new interface

op.set_attr<FExecType>("FExecType", OpExecType);
op.set_attr<FCreateOpState>("FCreateOpState", OpPropCreateLayerOp);
op.set_attr<FStatefulCompute>("FStatefulCompute<cpu>", LegacyOpForward);
op.set_attr<FStatefulCompute>("FStatefulCompute<gpu>", LegacyOpForward);
if (reg->key_var_num_args.length() != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to set attr for FStatefulComputeEx<xpu> for legacy ops? Do we have to modify the interface of operator class to have Forward/Backward with NDArray inputs & outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally you use FStatefulCompute

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so legacy op won't support sparse NDArrays - they'll all fallback to dense before forward/backward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. You need to move to new interface to add sparse support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

*
* \note Register under "FStatefulCompute<cpu>" and "FStatefulCompute<gpu>"
*/
using FStatefulCompute = std::function<void (const dmlc::any& state,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const reference gives user a feeling that state won't be mutated, maybe a pointer type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mutable state you need to use shared_ptr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but does this API implies that state can be mutated, or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state it's self cannot be mutated. If it's a pointer the content can be mutated.
The thing is state can be copied around and you cannot modify it inplace.

Another option is to use shared_ptr<any> state but that forces you to use a pointer and you cannot use make_shared to construct state inplace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be at least heavily documented in the comment

@piiswrong piiswrong closed this Jul 11, 2017
@piiswrong piiswrong reopened this Jul 11, 2017
Guneet-Dhillon pushed a commit to Guneet-Dhillon/mxnet that referenced this pull request Sep 13, 2017
* refactor create layer

* fix

* refactor custom op

* fix

* fix

* fix

* fix

* fix OpState

* remove superfluous infershape

* fix

* fix

* fix lint

* fix

* fix

* fix

* Update CMakeLists.txt

* delete

* fix

* fix scala
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants